-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-6366] Separate WebSocket max message size into a dedicated REST API #5099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
8a0cd93 to
001b3d6
Compare
001b3d6 to
ed1cae5
Compare
|
I don't know if we should also adapt zeppelin-web? What do you think? |
|
@Reamer Looks reasonable for zeppelin-web too. Are there specific concerns you’re considering, such as maintenance or behavior differences? |
|
@seung-00 zeppelin/zeppelin-web/src/app/app.js Line 170 in f1f6713
session_logout and programmatically clicks the login button (.nav-login-btn).As a result, the login modal pops up immediately when the window opens, which is different from before, and the integration test fails. It looks like we should change this to issue the API request only when the user is authenticated, or defer it until the moment it is actually needed. |
Yes both. All frontends should use the same interfaces. This makes it easier to remove things later on. |
de8726d to
3e5933e
Compare
|
@Reamer @tbonelee BTW, if the user isn’t logged in initially, showing the login modal seems more natural. 🤔 |
zeppelin-integration/src/test/java/org/apache/zeppelin/integration/ZeppelinIT.java
Show resolved
Hide resolved
tbonelee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems right but the unused imports should be removed.
Co-authored-by: ChanHo Lee <[email protected]>
Co-authored-by: ChanHo Lee <[email protected]>
tbonelee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I also agree with showing a login modal when authentication is required. Maybe we could address that in another issue.
Reamer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I don’t think we need to do that for every task. Covering both sides would be a bit tricky, since the front-end codebases are quite different.
Sooner or later, we should send the old front end into well-deserved retirement. In my opinion, sooner rather than later.
| import org.apache.zeppelin.service.AuthenticationService; | ||
|
|
||
| /** Configurations Rest API Endpoint. */ | ||
| @Path("/configurations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have changed the path of the API class here. Probably so that the call is /api/wsMaxMessageSize. However, I also think /api/configurations/wsMaxMessageSize is very good and would prefer this API endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Reamer
I also think the path you mentioned looks good. However, since the authentication referenced in shiro.ini appears to be set up based on the /configuration path, I excluded /configurations from the path. The wsMaxMessageSize API is intended to be a non-authenticated API.
Please feel free to let me know if you have any other suggestions. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Reamer
Would you mind taking a look at this?
The issue this PR addresses, configuration data being accessible via WebSocket without a permission check because of the wsMaxMessageSize field, seems to be a clear problem that needs to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @seung-00 ,
Thank you for bringing this issue to my attention again.
As I see it, we have the following options.
- We include the path
/api/configurations/wsMaxMessageSizeinshiro.iniand give unauthenticated users the right to call this endpoint.
This would be a breaking change. - We add a new path, but it would also have to contain something related to configuration in the path, e.g.,
/api/configuration/wsMaxMessageSize(withouts). The entire API endpoint would then also have to be mapped to a new class.
Theshiro.iniwould also have to be adjusted here, and that would also be a breaking change.
What bothers me about 1) is that potentially new frontend configuration options would also have to be added to the shiro.ini.
What bothers me about 2) is that it's quite confusing, but you probably only need to adjust the shiro.ini once.
What do you think about creating a general endpoint for frontend configurations? With this pull request, it's wsMaxMessageSize, tomorrow it might be something else. Then you could build the whole thing in the ConfigurationsRestApi and use frontend-config or something else as the endpoint path. You would only have to adjust shiro.ini once and extend the map that the endpoint returns.
Either way, this change would be a breaking change, since the shiro.ini, for example, is very variable.
See: https://github.com/apache/zeppelin/blob/master/conf/shiro.ini.template#L129-L130
What is this PR for?
Currently, configuration data is fetched through both REST API and WebSocket channels. However, the WebSocket path does not perform permission checks, and the only required data from it is the WebSocket max message size.
I extracted the websocket max message size field into a dedicated REST API, to improve security and simplify configuration handling.
What type of PR is it?
Improvement
Todos
What is the Jira issue?
How should this be tested?
Screenshots (if appropriate)
Questions: